Skip to content

[clang][Dependency Scanning] Move Module Timestamp Update After Compilation Finishes #151774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 6, 2025

Conversation

qiongsiwu
Copy link
Contributor

@qiongsiwu qiongsiwu commented Aug 1, 2025

When two threads are accessing the same pcm, it is possible that the reading thread sees the timestamp update, while the file on disk is not updated.

This PR moves timestamp update from writeAST to compileModuleAndReadASTImpl, so we only update the timestamp after the file has been committed to disk.

rdar://152097193

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Aug 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 1, 2025

@llvm/pr-subscribers-clang

Author: Qiongsi Wu (qiongsiwu)

Changes

When two threads are accessing the same pcm, it is possible that the reading thread sees the timestamp update, while the file on disk is not updated.

This PR moves timestamp update from writeAST to compileModuleAndReadASTImpl, so we only update the timestamp when the file is committed to disk.

rdar://152097193


Full diff: https://github.com/llvm/llvm-project/pull/151774.diff

2 Files Affected:

  • (modified) clang/lib/Frontend/CompilerInstance.cpp (+8)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (-5)
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index ed6a651d919a1..ae285b117f5e0 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1480,6 +1480,14 @@ static bool compileModuleAndReadASTImpl(CompilerInstance &ImportingInstance,
     return false;
   }
 
+  // The module is built successfully, we can update its timestamp now.
+  if (ImportingInstance.getPreprocessor()
+          .getHeaderSearchInfo()
+          .getHeaderSearchOpts()
+          .ModulesValidateOncePerBuildSession) {
+    ImportingInstance.getModuleCache().updateModuleTimestamp(ModuleFileName);
+  }
+
   return readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
                                    Module, ModuleFileName,
                                    /*OutOfDate=*/nullptr, /*Missing=*/nullptr);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index a6957e54b66f1..0c7ddd07553a4 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -5461,11 +5461,6 @@ ASTWriter::WriteAST(llvm::PointerUnion<Sema *, Preprocessor *> Subject,
 
   WritingAST = false;
 
-  if (WritingModule && PPRef.getHeaderSearchInfo()
-                           .getHeaderSearchOpts()
-                           .ModulesValidateOncePerBuildSession)
-    ModCache.updateModuleTimestamp(OutputFile);
-
   if (ShouldCacheASTInMemory) {
     // Construct MemoryBuffer and update buffer manager.
     ModCache.getInMemoryModuleCache().addBuiltPCM(

@llvmbot
Copy link
Member

llvmbot commented Aug 1, 2025

@llvm/pr-subscribers-clang-modules

Author: Qiongsi Wu (qiongsiwu)

Changes

When two threads are accessing the same pcm, it is possible that the reading thread sees the timestamp update, while the file on disk is not updated.

This PR moves timestamp update from writeAST to compileModuleAndReadASTImpl, so we only update the timestamp when the file is committed to disk.

rdar://152097193


Full diff: https://github.com/llvm/llvm-project/pull/151774.diff

2 Files Affected:

  • (modified) clang/lib/Frontend/CompilerInstance.cpp (+8)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (-5)
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index ed6a651d919a1..ae285b117f5e0 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1480,6 +1480,14 @@ static bool compileModuleAndReadASTImpl(CompilerInstance &ImportingInstance,
     return false;
   }
 
+  // The module is built successfully, we can update its timestamp now.
+  if (ImportingInstance.getPreprocessor()
+          .getHeaderSearchInfo()
+          .getHeaderSearchOpts()
+          .ModulesValidateOncePerBuildSession) {
+    ImportingInstance.getModuleCache().updateModuleTimestamp(ModuleFileName);
+  }
+
   return readASTAfterCompileModule(ImportingInstance, ImportLoc, ModuleNameLoc,
                                    Module, ModuleFileName,
                                    /*OutOfDate=*/nullptr, /*Missing=*/nullptr);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index a6957e54b66f1..0c7ddd07553a4 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -5461,11 +5461,6 @@ ASTWriter::WriteAST(llvm::PointerUnion<Sema *, Preprocessor *> Subject,
 
   WritingAST = false;
 
-  if (WritingModule && PPRef.getHeaderSearchInfo()
-                           .getHeaderSearchOpts()
-                           .ModulesValidateOncePerBuildSession)
-    ModCache.updateModuleTimestamp(OutputFile);
-
   if (ShouldCacheASTInMemory) {
     // Construct MemoryBuffer and update buffer manager.
     ModCache.getInMemoryModuleCache().addBuiltPCM(

@qiongsiwu
Copy link
Contributor Author

Gentle ping for review. Thanks!

Copy link
Collaborator

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me, but can we add a test case? It seems like it ought to be possible to catch this in a lit test, even if it would only reproduce rarely.

Copy link
Contributor

@Bigcheese Bigcheese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with or without a test for this specific timing issue.

@qiongsiwu
Copy link
Contributor Author

Thanks for the comments and reviews @benlangmuir and @Bigcheese!

Note that we have an existing functional test clang/test/Modules/fmodules-validate-once-per-build-session.c that validates the timestamp computation and validation logic. If I simply remove the timestamp code from writeAST, this test will fail. So in this sense we have some functional coverage.

I've discussed this with both @benlangmuir and @Bigcheese offline. It is not easy to do a lit test with a reasonable hit rate. We may be able to do a unit test, but that might be more involved. I will think about some ways to test race conditions later, and I will leave this PR as is.

@qiongsiwu qiongsiwu merged commit 09dbdf6 into llvm:main Aug 6, 2025
9 checks passed
qiongsiwu added a commit to qiongsiwu/llvm-project that referenced this pull request Aug 7, 2025
…lation Finishes (llvm#151774)

When two threads are accessing the same `pcm`, it is possible that the
reading thread sees the timestamp update, while the file on disk is not
updated.

This PR moves timestamp update from `writeAST` to
`compileModuleAndReadASTImpl`, so we only update the timestamp after the
file has been committed to disk.

rdar://152097193
(cherry picked from commit 09dbdf6)
qiongsiwu added a commit to swiftlang/llvm-project that referenced this pull request Aug 8, 2025
…lation Finishes (llvm#151774)

When two threads are accessing the same `pcm`, it is possible that the
reading thread sees the timestamp update, while the file on disk is not
updated.

This PR moves timestamp update from `writeAST` to
`compileModuleAndReadASTImpl`, so we only update the timestamp after the
file has been committed to disk.

rdar://152097193
(cherry picked from commit 09dbdf6)
qiongsiwu added a commit to qiongsiwu/llvm-project that referenced this pull request Aug 8, 2025
…lation Finishes (llvm#151774)

When two threads are accessing the same `pcm`, it is possible that the
reading thread sees the timestamp update, while the file on disk is not
updated.

This PR moves timestamp update from `writeAST` to
`compileModuleAndReadASTImpl`, so we only update the timestamp after the
file has been committed to disk.

rdar://152097193
(cherry picked from commit 09dbdf6)
qiongsiwu added a commit to swiftlang/llvm-project that referenced this pull request Aug 8, 2025
…lation Finishes (llvm#151774)

When two threads are accessing the same `pcm`, it is possible that the
reading thread sees the timestamp update, while the file on disk is not
updated.

This PR moves timestamp update from `writeAST` to
`compileModuleAndReadASTImpl`, so we only update the timestamp after the
file has been committed to disk.

rdar://152097193
(cherry picked from commit 09dbdf6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants